Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only log basic response information #3508

Merged
merged 2 commits into from
Feb 25, 2020

Conversation

JDutil
Copy link
Contributor

@JDutil JDutil commented Feb 6, 2020

Description

When logging a gateway error response there is a params field,
which provides extra information about the request. That information
can include useful information like the payment amount, but it can
also include PII such as a full billing address.

By logging the full error response in yaml that PII can end up in your
logs, which is not desirable and potentially against the law. Therefore
we should only log the minimal information needed from the response.

Checklist:

@JDutil JDutil force-pushed the jdutil/gateway-loggingg branch 2 times, most recently from 85a3823 to 9fdf9fd Compare February 7, 2020 22:03
@JDutil JDutil force-pushed the jdutil/gateway-loggingg branch from 9fdf9fd to b97da39 Compare February 7, 2020 22:05
When logging a gateway error response there is a params field,
which provides extra information about the request. That information
can include useful information like the payment amount, but it can
also include PII such as a full billing address.

By logging the full error response in yaml that PII can end up in your
logs, which is not desirable and potentially against the law. Therefore
we should only log the minimal information needed from the response.
@JDutil JDutil force-pushed the jdutil/gateway-loggingg branch from b97da39 to 63169fe Compare February 7, 2020 22:28
Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JDutil thanks for this improvement! I left a question regarding the specs.

core/spec/models/spree/payment_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JDutil, this makes sense!

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JDutil thank you 👍

@ericsaupe ericsaupe merged commit 0bbe459 into solidusio:master Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants